Skip to content

DOC: Fix to lzma.LZMAFile #30010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Dec 4, 2019
Merged

DOC: Fix to lzma.LZMAFile #30010

merged 5 commits into from
Dec 4, 2019

Conversation

ShaharNaveh
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

jbrockmendel
jbrockmendel reviewed Dec 3, 2019
View reviewed changes
pandas/compat/__init__.py Outdated
@@ -123,7 +123,7 @@ def _get_lzma_file(lzma):

Returns
-------
class
lzma.LZMAFile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly type(LZMAFile)? usually this would indicate that we're getting an instance of that class

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should annotate the function return here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should annotate the function return here as well

I thought of that. but the problem is that the function tests for the imports of the lzma module. If I import it beforehand it kinda losing it's point (I think)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should annotate the function return here as well

@WillAyd I tried it now, I imported the lzma module at the beginning of the file and annotated _get_lzma_file with -> lzma.LZMAFile, and mypy complained:

pandas/io/common.py:468: error: "LZMAFile" not callable

Any thoughts?

WillAyd
WillAyd requested changes Dec 4, 2019
View reviewed changes
pandas/compat/__init__.py Outdated
@@ -123,7 +123,7 @@ def _get_lzma_file(lzma):

Returns
-------
class
lzma.LZMAFile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should annotate the function return here as well

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Dec 4, 2019
@jreback jreback added this to the 1.0 milestone Dec 4, 2019
@WillAyd
Copy link
Member

WillAyd commented Dec 4, 2019

It would need to be Type[lzma.LZMAFile], though not sure if we can do that given this is technically an optional dependency from the stdlib

@WillAyd
Copy link
Member

WillAyd commented Dec 4, 2019

Does Type[IO] work? May be the best bet

@topper-123
Copy link
Contributor

topper-123 commented Dec 4, 2019 •
edited
Loading

LZMA has been part of the standard library since python 3.3. We should just assume it’s installed, that will simplify this a bit.

@jbrockmendel
Copy link
Member

LZMA has been part of the standard library since python 3.3. We should just remove assume it’s installed, that will simplify this a bit.

I'm pretty sure that just recently i had to do an apt-get install to get this working on an ubuntu 18 machine

@WillAyd
Copy link
Member

WillAyd commented Dec 4, 2019

We had an issue previously with that assumption, hence the existence of this compat to begin with. See #27575

@topper-123
Copy link
Contributor

Ok, that’s surprising. Yeah, changing it isn’t possible then, I guess...

@ShaharNaveh
Copy link
Member Author

It would need to be Type[lzma.LZMAFile], though not sure if we can do that given this is technically an optional dependency from the stdlib

mypy's log:

Success: no issues found in 808 source files

Does Type[IO] work? May be the best bet

mypy's log:

pandas/io/common.py:468: error: Too many arguments for "IO"
Found 1 error in 1 file (checked 808 source files)

@ShaharNaveh
Copy link
Member Author

It would need to be Type[lzma.LZMAFile], though not sure if we can do that given this is technically an optional dependency from the stdlib

mypy's log:

Success: no issues found in 808 source files

Does Type[IO] work? May be the best bet

mypy's log:

pandas/io/common.py:468: error: Too many arguments for "IO"
Found 1 error in 1 file (checked 808 source files)

I just found a bug (I think), unless I'm missing something obvious.

When _get_lzma_file is annotated with -> Type[IO], normally mypy will say:

pandas/io/common.py:468: error: Too many arguments for "IO"
Found 1 error in 1 file (checked 808 source files)

But if the lzma module is imported at the beginning of the file you mypy will say:

Success: no issues found in 808 source files

So two questions actually:

  • Is this a bug?
  • What option do we prefer? (if any)

@WillAyd WillAyd merged commit 04c0870 into pandas-dev:master Dec 4, 2019
@WillAyd
Copy link
Member

WillAyd commented Dec 4, 2019

Thanks @MomIsBestFriend

Can come back to types later for the whole module if something you are interested in

@ShaharNaveh ShaharNaveh deleted the TYP-compact branch December 5, 2019 10:22
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@ShaharNaveh @proost
DOC: Fix to lzma.LZMAFile (pandas-dev#30010)
acb5488
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
@ShaharNaveh @proost
DOC: Fix to lzma.LZMAFile (pandas-dev#30010)
501fa91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@WillAyd WillAyd WillAyd approved these changes

Assignees
No one assigned
Labels
Docs Typing type annotations, mypy/pyright type checking
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ShaharNaveh @WillAyd @topper-123 @jbrockmendel @jreback @alimcmaster1